fix(desktop): oauth popups open from the browser preview - #8435
fix(desktop): oauth popups open from the browser preview#8435walid-baharwal wants to merge 3 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
github-actions
Bot
added
vouch:unvouched
Scripted `window.open` calls inside the integrated browser preview were denied and loaded in the preview tab instead. Firebase `signInWithPopup` got a null window handle back and reported `auth/popup-blocked`, and the in-tab load also dropped the opener the popup needs to post the credential back to. Popups with a `new-window` disposition and an http or https URL now get a real window, with context isolation and the sandbox turned back on: a popup is not a webview attach, so the `will-attach-webview` hardening never sees it and an unoverridden child would inherit the picker preload's relaxed posture. `about:blank` popups keep loading in the preview tab, since Chromium copies the guest preferences for them and forbids overriding. Links with `target="_blank"` are unchanged. Fixes pingdotgg#6561
e598878 to
8fa05ae
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
An allowed popup carried Electron's default window-open behavior, so a page inside it could spawn native windows without limit. The popup now denies its own window.open calls; no OAuth flow opens a second popup. The popup preferences also drop nodeIntegrationInSubFrames, matching the three keys every other hardened window in the app sets.
d2ce056 to
7c5b69a
Compare
Electron reads allowpopups when the guest attaches. The attribute was set from the ref callback, which runs after the element is in the DOM, so every preview guest attached with popups disabled and Electron blocked window.open before the window-open handler ran. Verified in a running desktop build: will-attach-webview reported allowpopups: false before this change and true after.
There was a problem hiding this comment.
One finding in the web scope: the new allowpopups JSX attribute is the right ownership fix, but its string value conflicts with React's element typing, so apps/web typecheck (tsgo --noEmit) breaks. Details inline.
Posted via Macroscope — UI Consistency
| // Must be an attribute on the element itself: Electron reads it when the | ||
| // guest attaches, so setting it from the ref callback lands too late and | ||
| // the guest attaches with popups disabled. | ||
| allowpopups="true" |
There was a problem hiding this comment.
Moving allowpopups onto the element is the correct owner for this behavior, but the value breaks the typing contract: @types/react declares WebViewHTMLAttributes.allowpopups?: boolean, so allowpopups="true" fails apps/web's tsgo --noEmit. Switching to allowpopups={true} typechecks but silently regresses the fix — react-dom drops boolean values for unrecognized attributes on non-custom tags (it warns "Received true for a non-boolean attribute" and sets no attribute), so the guest would attach with popups disabled again.
Suggest keeping the string value and spreading it past the boolean type (same shape already used below for preload):
| // Must be an attribute on the element itself: Electron reads it when the | |
| // guest attaches, so setting it from the ref callback lands too late and | |
| // the guest attaches with popups disabled. | |
| allowpopups="true" | |
| // Must be an attribute on the element itself: Electron reads it when the | |
| // guest attaches, so setting it from the ref callback lands too late and | |
| // the guest attaches with popups disabled. React types `allowpopups` as a | |
| // boolean, but react-dom drops boolean values for unrecognized attributes, | |
| // so the literal string has to be spread past the type. | |
| {...({ allowpopups: "true" } as unknown as { readonly allowpopups?: boolean })} |
Posted via Macroscope — UI Consistency
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0604de5. Configure here.
| nodeIntegration: false, | ||
| sandbox: true, | ||
| }, | ||
| } satisfies Electron.BrowserWindowConstructorOptions; |
There was a problem hiding this comment.
Popup may inherit picker preload
High Severity
POPUP_WINDOW_OPTIONS overrides isolation flags but never clears preload. Electron merges overrideBrowserWindowOptions with the guest's preferences, so the picker preload can still run in the OAuth window and observe keystrokes and clicks on a third-party login page.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0604de5. Configure here.
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR changes production webview and BrowserWindow behavior to enable OAuth popups, including security-sensitive preload and isolation handling for third-party login pages. Unresolved concerns remain about picker-preload inheritance in popup windows and the You can add or adjust custom eligibility rules. Learn more. |
1 participant
What Changed
Two changes, both needed for an OAuth popup to work in the integrated browser preview.
apps/web/src/browser/HostedBrowserWebview.tsxsetsallowpopupsas an attribute on the<webview>element instead of from the ref callback. Electron reads that flag when the guest attaches, and the ref callback runs after the element is already in the DOM, so every preview guest attached with popups disabled.apps/desktop/src/preview/Manager.tsreads thedispositionthe window-open handler already received. Anew-windowdisposition with an http or https URL now opens a real window; everything else,target="_blank"links included, keeps loading in the preview tab as before. The popup is created withcontextIsolation,sandbox, andnodeIntegration: falseset explicitly, since a popup is not a webview attach and never passes thewill-attach-webviewhardening inDesktopWindow. It also gets a deny-only window-open handler of its own, so a page inside it cannot spawn native windows without limit.about:blankpopups keep loading in the preview tab: Chromium copies the guest preferences for them and gives no way to override.Why
A local app opened in the preview cannot finish an OAuth popup flow. Firebase
signInWithPopup(auth, new GoogleAuthProvider())reportsauth/popup-blockedand no window appears, while the same app works in a normal Chrome or Firefox window.Two separate causes stack up.
window.openwas denied and the URL was force-loaded into the same webContents, sowindow.open()returnednull(the SDK reads that as a blocked popup) and the in-tab load destroyed the opener the popup needs topostMessageits credential back to. Underneath that, the guest attached withallowpopupsfalse, so Electron blocked the call before the handler ran at all.Instrumenting
will-attach-webviewin a running desktop build showed it directly:Surfaces
Desktop only in behavior. The
<webview>element lives inapps/webbecause the desktop app wraps the web client, but the tag only exists inside Electron and remote web previews never reachsetWindowOpenHandler. No contract, provider, or docs change.UI Changes
No layout, styling, or motion changed. The observable difference is whether a popup window exists, captured below in a dev build against a local page that calls
window.open(url, name, "width=520,height=640"), the same shapesignInWithPopupuses.Before
window.open()returns null and no window opens.After
window.open()returns a window handle.The popup window itself, sized from the requested window features:
Verification
Run in a dev desktop build (
dev:desktop) with the preview open on a local page:window.open(...)returns a handle. Handler loggeddisposition: "new-window"and decidedpopup.window.openerpresent andtypeof require === "undefined", so the opener survives and the hardening applied.window.openfrom inside the popup returnsnull.target="_blank"link logsdisposition: "foreground-tab", decidesnavigate, and loads in the preview tab.previewWindowOpenActionhas focused unit tests next toisPreviewRefreshShortcut, the existing pure helper in the same file.vp test run apps/desktop/src/preview/Manager.test.tshas not run on my machine:node-ptyhas no linux-x64 prebuild and this box has no C++ compiler, so the workspace never fully built for tests. CI is the first full run, and I will fix whatever it reports.The attach-timing half is not unit-testable in a meaningful way. A rendered-DOM assertion passes either way, because the ref callback does set the attribute, just too late for Electron to read it.
Checklist
Fixes #6561
Written with Claude Opus 5 in Claude Code.
Note
Medium Risk
Changes Electron window-open and webview security boundaries for preview content; mitigations are protocol allowlisting and explicit popup hardening, but third-party pages can still open http(s) popups.
Overview
Fixes OAuth popup flows (e.g. Firebase
signInWithPopup) in the integrated browser preview by addressing two stacked blockers.HostedBrowserWebviewnow setsallowpopups="true"on the<webview>element at render time instead of in a ref callback, so Electron sees popups enabled when the guest attaches.PreviewManageraddspreviewWindowOpenAction: scriptedwindow.openwith dispositionnew-windowand an http/https URL opens a real BrowserWindow with hardenedwebPreferences(contextIsolation,sandbox, no Node).target="_blank"links still load in the preview tab. Unsafe or non-overridable URLs (about:blank,javascript:,file:, deep links, etc.) stay in-tab. Child popups get a deny-allsetWindowOpenHandlerviadid-create-window.Unit tests cover
previewWindowOpenActiondecision logic.Reviewed by Cursor Bugbot for commit 0604de5. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix OAuth popups opening from browser preview
webContentsnow opens real popupBrowserWindows for eligible http/https scripted popups viapreviewWindowOpenActionin Manager.ts; other dispositions or URLs are denied and loaded in the current tab instead.webPreferences(contextIsolation,sandbox) and a secondarysetWindowOpenHandlerthat always denies further popups.allowpopupsattribute directly on the<webview>element in HostedBrowserWebview.tsx so Electron reads it at attach time.makeNativeOperationsreplaces the previous deny-and-navigate open handler; non-http(s) schemes that previously navigated the current tab still do, but callers relying on popups being globally blocked will now see real windows for http/httpsnew-windowrequests.Macroscope summarized 0604de5.